Move subscription methods up a level #618

stephenplusplus · 2015-05-20T19:34:47Z

Fixes #605

lib/pubsub/index.js

+ *
+ * @throws {Error} If a name is not provided.
+ *
+ * @param {string} name - The name of the subscription.


ryanseys · 2015-05-20T20:52:48Z

Different than what I had in mind.

I was thinking more like:

// topic is in options
pubsub.subscribe('my-sub', { topic: pubsub.topic('some-topic') }, callback);

Thinking now, pubsub.subscribe('sub-name', topicObj, options, callback) makes more sense because its not an option in this case BUT a topic name is specified by its project and topic name like so: 'projects/project-id/topics/topic-name' so with your string version here, that wouldn't be the best unless that assumes that the topic is in the same project. It would be better to pass in a whole topic object.

Here's a partial attempt I made a while ago: https://github.com/ryanseys/gcloud-node/commit/78c4cfe6b06a7dcc1d44e226b00dbbc51a31d096 don't let it confuse you because it's not complete but something to gander at.

stephenplusplus · 2015-05-21T12:32:08Z

BUT a topic name is specified by its project and topic name like so: 'projects/project-id/topics/topic-name

I'm just getting the topic name (topic-name), then if the user gives a project-id, that project-id is used to make the full topic name, otherwise, use what's on the PubSub instance.

Quick API comparison :)

// ---
// This PR:
// ---
var pubsub = gcloud.pubsub({ projectId: 'grape-spaceship-123' });

// Each of these next two lines do the same thing:
pubsub.topic('some-topic').subscribe('my-sub', callback);
pubsub.subscribe('some-topic', 'new-sub-name', callback);

// Subscribe to a topic from another project.
pubsub.subscribe('another-topic', 'another-new-sub-name', { projectId: 'squishy-butter-334' }, callback);

// ---
// Your Idea:
// ---
var pubsub = gcloud.pubsub({ projectId: 'grape-spaceship-123' });

// Each of these next two lines do the same thing:
pubsub.topic('some-topic').subscribe('my-sub', callback);
pubsub.subscribe('new-sub-name', pubsub.topic('some-topic'), callback);

// Subscribe to a topic from another project.
var anotherPubsub = gcloud.pubsub({ projectId: 'squishy-butter-334' });

pubsub.subscribe('another-new-sub-name', anotherPubsub.topic('another-topic'), callback);

The main difference is requiring a Topic object vs. using explicit string params. I like both. Halp.

jgeewax · 2015-05-21T12:41:35Z

Couldn't we take bits of both?

var pubsub1 = gcloud.pubsub({ projectId: 'project1' });

// These all do the same thing
pubsub1.topic('some-topic').subscribe('my-sub', callback); // Most common pattern
pubsub1.subscribe('new-sub-name', 'some-topic', callback);  // Uncommon, but just a name implies same project
pubsub1.subscribe('new-sub-name', pubsub1.topic('some-topic'), callback); // Uncommon, most explicit.

// So then, if we want to subscribe to a topic in another project,
// the only way to do that is to create a topic object that knows it's project.
var pubsub2 = gcloud.pubsub({ projectId: 'project2' });
pubsub2.subscribe('new-sub-name', pubsub1.topic('some-topic'), callback);

stephenplusplus · 2015-05-21T12:55:09Z

I'm cool with that. Now just one smaller detail; shouldn't topic name come before subscription?

pubsub.subscribe('new-sub-name', 'some-topic', callback);
// vs.
pubsub.subscribe('some-topic', 'new-sub-name', callback);

I like the latter, just to follow the standard hierarchy "project -> topic -> subscription".

Just for completeness, here's the full sig:

pubsub.subscribe( ( Topic('topic-name') || 'topic-name' ), 'sub-name', [{ autoAck: true, ... }], function(err, subscription, apiResponse) {});

jgeewax · 2015-05-21T12:55:58Z

SGTM. Since topic is absolutely required, it should definitely be first.

ryanseys · 2015-05-21T16:17:23Z

Since topic is absolutely required, it should definitely be first.

Aren't both topic and sub-name required? Either order makes sense to me, so whatever you like :)

jgeewax · 2015-05-21T16:18:04Z

Yea, both required -- topic being higher up in the hierarchy seems like it should come first though. Sorry, didn't articulate that piece too. :P

ryanseys · 2015-05-21T16:32:18Z

Cool SGTM!

stephenplusplus · 2015-05-21T16:49:08Z

PTAL.

lib/pubsub/index.js

@@ -163,6 +163,123 @@ PubSub.prototype.createTopic = function(name, callback) {
 };

 /**
+ * Create a subscription to this topic. You may optionally provide an object to


stephenplusplus · 2015-06-02T17:22:21Z

While Travis takes a year to run this build...

I'm not sure how I can write a system test for this functionality, since we only have access to one project. Unit testing covers all of the changes pretty well, since it was mostly just relocating code. To support subscribing from another project, all we're really doing is passing a different topic string to the API. The unit tests cover that as well. Is that enough?

ryanseys · 2015-06-03T20:16:24Z

I think this should be good. I can go ahead with a code review if you're ready?

stephenplusplus · 2015-06-03T23:59:01Z

Yes, please!

lib/pubsub/index.js

+
+ options = options || {};
+
+ if (util.is(topic, 'string')) {


lib/common/util.js

@@ -124,7 +124,7 @@ util.arrayize = arrayize;
 */
 function format(template, args) {
 return template.replace(/{([^}]*)}/g, function(match, key) {
- return args[key] || match;
+ return key in args ? args[key] : match;


stephenplusplus · 2015-06-05T16:32:24Z

Merge time!

Move subscription methods up a level

#618) * build(node): add feat in node post-processor to add client library version number in snippet metadata Co-authored-by: Benjamin E. Coe <bencoe@google.com> Source-Link: googleapis/synthtool@d337b88 Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-nodejs:latest@sha256:d106724ad2a96daa1b8d88de101ba50bdb30b8df62ffa0aa2b451d93b4556641

🤖 I have created a release \*beep\* \*boop\* --- ### [3.4.1](https://www.github.com/googleapis/nodejs-video-intelligence/compare/v3.4.0...v3.4.1) (2021-09-15) ### Bug Fixes * **deps:** require google-gax v2.24.1 ([#589](https://www.github.com/googleapis/nodejs-video-intelligence/issues/589)) ([6af1a84](https://www.github.com/googleapis/nodejs-video-intelligence/commit/6af1a84112ab56a3ff2862ff7a5eda0da9772244)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

This PR was generated using Autosynth. 🌈 Synth log will be available here: https://source.cloud.google.com/results/invocations/1bcecc39-aa5c-40f4-9453-955704c7ca01/targets - [ ] To automatically regenerate this PR, check this box. Source-Link: googleapis/synthtool@57c23fa

- [ ] Regenerate this pull request now. PiperOrigin-RevId: 468790263 Source-Link: googleapis/googleapis@873ab45 Source-Link: googleapis/googleapis-gen@cb6f37a Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiY2I2ZjM3YWVmZjJhMzQ3MmU0MGE3YmJhY2U4YzY3ZDc1ZTI0YmVlNSJ9

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>

#618) * build(node): add feat in node post-processor to add client library version number in snippet metadata Co-authored-by: Benjamin E. Coe <bencoe@google.com> Source-Link: googleapis/synthtool@d337b88 Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-nodejs:latest@sha256:d106724ad2a96daa1b8d88de101ba50bdb30b8df62ffa0aa2b451d93b4556641

* feat: support regapic LRO Use gapic-generator-typescript v2.15.1. PiperOrigin-RevId: 456946341 Source-Link: googleapis/googleapis@88fd18d Source-Link: googleapis/googleapis-gen@accfa37 Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiYWNjZmEzNzFmNjY3NDM5MzEzMzM1YzY0MDQyYjA2M2MxYzUzMTAyZSJ9 * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>

stephenplusplus added api: pubsub Issues related to the Pub/Sub API. don't merge labels May 20, 2015

googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 20, 2015

ryanseys reviewed May 20, 2015
View reviewed changes

lib/pubsub/index.js

*

* @throws {Error} If a name is not provided.

*

* @param {string} name - The name of the subscription.

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

stephenplusplus force-pushed the spp--pubsub-level-methods branch from 1a12ede to 82d316b Compare May 21, 2015 16:47

ryanseys reviewed May 21, 2015
View reviewed changes

lib/pubsub/index.js

@@ -163,6 +163,123 @@ PubSub.prototype.createTopic = function(name, callback) {

};

/**

* Create a subscription to this topic. You may optionally provide an object to

This comment was marked as spam.

Sign in to view

stephenplusplus force-pushed the spp--pubsub-level-methods branch 6 times, most recently from 6c95ee7 to 08ed5d4 Compare May 29, 2015 19:39

stephenplusplus force-pushed the spp--pubsub-level-methods branch 2 times, most recently from 3c59635 to a4e3243 Compare June 2, 2015 17:01

ryanseys reviewed Jun 4, 2015
View reviewed changes

lib/pubsub/index.js

options = options || {};

if (util.is(topic, 'string')) {

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

move subscription methods up a level

0669289

stephenplusplus force-pushed the spp--pubsub-level-methods branch from a4e3243 to 0669289 Compare June 4, 2015 13:52

stephenplusplus removed the don't merge label Jun 4, 2015

odeke-em reviewed Jun 5, 2015
View reviewed changes

stephenplusplus added a commit that referenced this pull request Jun 5, 2015

Merge pull request #618 from stephenplusplus/spp--pubsub-level-methods

f167223

Move subscription methods up a level

stephenplusplus merged commit f167223 into googleapis:master Jun 5, 2015

sofisl mentioned this pull request Sep 27, 2022

migrate code from googleapis/nodejs-video-intelligence #3374

Closed

4 tasks

This was referenced Nov 9, 2022

migrate code from googleapis/nodejs-video-intelligence #3507

Closed

migrate code from googleapis/nodejs-video-intelligence #3519

Merged

migrate code from googleapis/nodejs-translate #3520

Merged

sofisl mentioned this pull request Nov 10, 2022

migrate code from googleapis/nodejs-text-to-speech #3528

Merged

4 tasks

release-please bot mentioned this pull request Nov 10, 2022

chore: release main #3529

Merged

sofisl mentioned this pull request Nov 11, 2022

migrate code from googleapis/nodejs-monitoring #3545

Merged

4 tasks

sofisl pushed a commit that referenced this pull request Nov 11, 2022

chore(main): release 3.0.0 (#618)

d0b21ae

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>

sofisl mentioned this pull request Nov 11, 2022

migrate code from googleapis/nodejs-asset #3567

Merged

4 tasks

sofisl pushed a commit that referenced this pull request Nov 17, 2022

feat: add minimum wrapper for compute LRO method (#618)

ea9a347

sofisl mentioned this pull request Nov 17, 2022

migrate code from googleapis/nodejs-compute #3666

Merged

4 tasks

takeratta mentioned this pull request Jul 6, 2023

[Snyk] Security upgrade @google-cloud/logging from 1.2.0 to 6.0.0 takeratta/google-cloud-node#4

Open

sofisl mentioned this pull request Sep 13, 2023

migrate code from googleapis/nodejs-automl #4655

Closed

4 tasks

sofisl mentioned this pull request Sep 14, 2023

migrate code from googleapis/nodejs-automl #4656

Merged

4 tasks

takeratta mentioned this pull request Nov 28, 2023

[Snyk] Fix for 21 vulnerabilities takeratta/google-cloud-node#28

Open

WontonSam mentioned this pull request Sep 17, 2024

[Snyk] Upgrade @google-cloud/sql from 0.8.0 to 0.18.0 WontonSam/Bekki13-cloud-node#2708

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move subscription methods up a level #618

Move subscription methods up a level #618

stephenplusplus commented May 20, 2015

This comment was marked as spam.

This comment was marked as spam.

ryanseys commented May 20, 2015

stephenplusplus commented May 21, 2015

jgeewax commented May 21, 2015

stephenplusplus commented May 21, 2015

jgeewax commented May 21, 2015

ryanseys commented May 21, 2015

jgeewax commented May 21, 2015

ryanseys commented May 21, 2015

stephenplusplus commented May 21, 2015

This comment was marked as spam.

stephenplusplus commented Jun 2, 2015

ryanseys commented Jun 3, 2015

stephenplusplus commented Jun 3, 2015

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

stephenplusplus commented Jun 5, 2015

Move subscription methods up a level #618

Move subscription methods up a level #618

Conversation

stephenplusplus commented May 20, 2015

This comment was marked as spam.

This comment was marked as spam.

ryanseys commented May 20, 2015

stephenplusplus commented May 21, 2015

jgeewax commented May 21, 2015

stephenplusplus commented May 21, 2015

jgeewax commented May 21, 2015

ryanseys commented May 21, 2015

jgeewax commented May 21, 2015

ryanseys commented May 21, 2015

stephenplusplus commented May 21, 2015

This comment was marked as spam.

stephenplusplus commented Jun 2, 2015

ryanseys commented Jun 3, 2015

stephenplusplus commented Jun 3, 2015

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

stephenplusplus commented Jun 5, 2015